Map [MinLength]/[MaxLength] on dictionary properties to minProperties / maxProperties#3922
Open
KitKeen wants to merge 5 commits intodomaindrivendev:masterfrom
Open
Map [MinLength]/[MaxLength] on dictionary properties to minProperties / maxProperties#3922KitKeen wants to merge 5 commits intodomaindrivendev:masterfrom
KitKeen wants to merge 5 commits intodomaindrivendev:masterfrom
Conversation
…erties / maxProperties Fixes domaindrivendev#3915. OpenAPI defines minLength/maxLength only for string schemas; for object schemas (including dictionaries) the equivalent constraints are minProperties/maxProperties. Before this change, applying [MinLength(1)] to a property typed as IReadOnlyDictionary<TKey, TValue> produced "minLength": 1 on the schema, which is invalid per the spec and is flagged by linters such as vacuum. The three attribute handlers (MinLength, MaxLength, Length) already switch to MinItems/MaxItems when the schema is an Array. Extend the same branching to recognise Object schemas and emit MinProperties/MaxProperties instead of MinLength/MaxLength. Route constraint handlers (MinLengthRouteConstraint, MaxLengthRouteConstraint) are intentionally left alone: route parameters are primitives and cannot be dictionaries, so the existing two-branch array/string logic is correct there. Tests added in OpenApiSchemaExtensionsTests: - MinLength on dictionary → minProperties - MaxLength on dictionary → maxProperties - Length on dictionary → min+maxProperties - Regression guards: MinLength on string still maps to minLength, on array still maps to minItems.
Added: - End-to-end coverage via shared TypeWithValidationAttributes fixture — DictionaryWithMinMaxLength ([MinLength(1),MaxLength(3)]) and DictionaryWithLength ([Length(1,3)]) properties, asserted by both JsonSerializerSchemaGenerator and NewtonsoftSchemaGenerator tests. Reproduces the exact scenario from the issue (dictionary property on a model class) rather than only exercising ApplyValidationAttributes in isolation. - Enum-keyed dictionary shape — Object schema with known Properties and AdditionalPropertiesAllowed=false (see CreateDictionarySchema). MinLength must still route to MinProperties here. - Nullable dictionary shape — Type = Object | Null. HasFlag(Object) must still route to MinProperties. - Symmetry regression guards: MaxLength/Length on string map to MinLength/MaxLength, on array map to MinItems/MaxItems (previously only MinLength had explicit guards). Fixture updates are picked up by both Newtonsoft and JsonSerializer generator test suites because the shared TestSupport fixture drives both.
Collaborator
martincostello
left a comment
There was a problem hiding this comment.
Please make some test changes to that the OpenAPI snapshots demonstrate the required changes to the generated OpenAPI schemas.
| [Fact] | ||
| public static void ApplyValidationAttributes_MinLength_On_Dictionary_Maps_To_MinProperties() | ||
| { | ||
| // Arrange — dictionary schema is represented as an Object with AdditionalProperties |
Collaborator
There was a problem hiding this comment.
Please remove all the AI —s and replace with -s.
|
|
||
| public string[] ArrayWithMinMaxLength { get; set; } | ||
|
|
||
| public IReadOnlyDictionary<string, string> DictionaryWithMinMaxLength { get; set; } |
Collaborator
There was a problem hiding this comment.
This name doesn't seem to match the purpose?
|
|
||
| public string[] ArrayWithLength { get; set; } | ||
|
|
||
| public Dictionary<string, string> DictionaryWithLength { get; set; } |
Collaborator
There was a problem hiding this comment.
This name doesn't seem to match the purpose?
- Replace em-dashes with hyphens in OpenApiSchemaExtensionsTests.cs - Rename fixture property DictionaryWithMinMaxLength to IReadOnlyDictionaryWithMinMaxLength so the name matches its actual IReadOnlyDictionary<,> declared type - Add JSON-output test that serializes the schema for TypeWithValidationAttributes/TypeWithValidationAttributesViaMetadataType to OpenAPI 3.0 JSON and asserts the dictionary properties surface as minProperties/maxProperties in the produced OpenAPI document
…ttribute Per review: 'Length' as a noun does not describe a dictionary (which has a property/item count, not a length). The Attribute suffix makes it explicit that the property exists to test the [Length] attribute mapping rather than a conceptual 'length' of the dictionary.
|
|
||
| public string[] ArrayWithLength { get; set; } | ||
|
|
||
| public Dictionary<string, string> DictionaryWithLengthAttribute { get; set; } |
Collaborator
There was a problem hiding this comment.
But it doesn't have an attribute...
|
|
||
| public string[] ArrayWithMinMaxLength { get; set; } | ||
|
|
||
| public IReadOnlyDictionary<string, string> IReadOnlyDictionaryWithMinMaxLength { get; set; } |
Collaborator
There was a problem hiding this comment.
And there's no min or max.
Per follow-up review: in TypeWithValidationAttributesViaMetadataType the outer class is a bare property list (attributes live on the inner MetadataType class), so names that referenced attributes were misleading. Renamed to neutral, type-focused names that work in both fixture variants: - IReadOnlyDictionaryWithMinMaxLength -> BoundedReadOnlyDictionary - DictionaryWithLengthAttribute -> BoundedDictionary The bounded prefix describes the test purpose (a dictionary whose size is constrained); the readonly/mutable distinction reflects the actual type under test. No attribute presence is claimed by the property name.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #3915.
Problem
Applying
[MinLength(1)]to a property typed as a dictionary (IDictionary<,>,IReadOnlyDictionary<,>, etc.) produces"minLength": 1on the generated schema:"minLength"on an object schema is invalid per the OpenAPI 3.x spec — it's defined only for string schemas. Linters such asvacuumflag this (the original reporter hit it exactly that way). The equivalent constraint for object schemas is"minProperties".Fix
OpenApiSchemaExtensions.Apply{Min,Max,}LengthAttributealready branches onschema.Typeto pickMinItems/MaxItemsfor arrays. This PR adds a parallel branch forJsonSchemaTypes.Objectso dictionary schemas getMinProperties/MaxPropertiesinstead of the meaninglessMinLength/MaxLength.No changes to the
MinLengthRouteConstraint/MaxLengthRouteConstrainthandlers — route parameters are primitives and cannot be dictionaries, so the existing Array/otherwise split is correct there.Before / after
Before:
After:
Tests
Added to
OpenApiSchemaExtensionsTests:MinLengthon dictionary schema →minPropertiesMaxLengthon dictionary schema →maxPropertiesLength(min,max)on dictionary schema →min+maxPropertiesMinLengthon string schema still maps tominLength; on array schema still maps tominItems.Scope
TypeincludesObject— previously those would receiveMinLength/MaxLength, which was invalid per the OpenAPI spec, so any consumer relying on the old output was already producing non-conforming documents.Checklist